Skip to content

chore(NODE-6844): modernize defer and remove incorrect usages#4456

Merged
baileympearson merged 2 commits intomainfrom
NODE-6844-defer-fixes
Mar 12, 2025
Merged

chore(NODE-6844): modernize defer and remove incorrect usages#4456
baileympearson merged 2 commits intomainfrom
NODE-6844-defer-fixes

Conversation

@nbbeeken
Copy link
Copy Markdown
Contributor

@nbbeeken nbbeeken commented Mar 11, 2025

Description

What is changing?

  • Add validation to defer to prevent incorrect usages
  • Use async/await to get stack traces
  • remove incorrect usages
Is there new documentation needed for these changes?

What is the motivation for this change?

The defer usages were passed a promise instead of a functiont that creates a promise.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken requested a review from a team as a code owner March 11, 2025 22:43
@baileympearson
Copy link
Copy Markdown
Contributor

baileympearson commented Mar 12, 2025

I think you mentioned over slack yesterday that the changes in this PR were motivated by a flaky change stream test. Is that correct?

Was this the test?

   1) Change Streams
        should properly handle a changeStream event being processed mid-close
          when invoked with callbacks:

If so, it is still failing in CI.

@baileympearson baileympearson self-assigned this Mar 12, 2025
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Mar 12, 2025
@nbbeeken
Copy link
Copy Markdown
Contributor Author

nbbeeken commented Mar 12, 2025

https://mongodb.enterprise.slack.com/archives/GGWBN4ZNK/p1741725536756809?thread_ts=1741723927.759519&cid=GGWBN4ZNK

Similar but a different one

@nbbeeken nbbeeken requested a review from baileympearson March 12, 2025 14:45
@nbbeeken nbbeeken force-pushed the NODE-6844-defer-fixes branch from 929562a to 93a24b2 Compare March 12, 2025 15:50
baileympearson
baileympearson suggested changes Mar 12, 2025
View reviewed changes
Comment thread test/integration/change-streams/change_stream.test.ts Outdated
Comment thread test/integration/change-streams/change_stream.test.ts Outdated
Comment thread test/integration/change-streams/change_stream.test.ts Outdated
@nbbeeken nbbeeken requested a review from baileympearson March 12, 2025 17:59
baileympearson
baileympearson approved these changes Mar 12, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flaky tests tracked in other PRs

@baileympearson baileympearson merged commit 1f5b716 into main Mar 12, 2025
@baileympearson baileympearson deleted the NODE-6844-defer-fixes branch March 12, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@baileympearson baileympearson baileympearson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@baileympearson baileympearson

Labels

Primary Review In Review with primary reviewer, not yet ready for team's eyes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nbbeeken @baileympearson